Conversation
|
I am fine with this change as a short-term fix, but wouldn't it better to fix the layout of CLI |
kjdelisle
left a comment
kjdelisle
left a comment
There was a problem hiding this comment.
If the CLI tests aren't following our monorepo layout, that tells me something fishy is going on...
| "pretest": "npm run build:current", | ||
| "test": "node packages/build/bin/run-nyc npm run mocha", | ||
| "mocha": "node packages/build/bin/select-dist mocha --opts test/mocha.opts \"packages/*/DIST/test/**/*.js\"", | ||
| "mocha": "node packages/build/bin/select-dist mocha --opts test/mocha.opts \"packages/*/DIST/test/**/*.js\" \"packages/cli/test\"", |
There was a problem hiding this comment.
Why is there no dist folder for the CLI tests? It should follow the existing pattern in the current command (packages/cli/dist/test/**/*.js)
There was a problem hiding this comment.
@kjdelisle I think the CLI cannot use TypeScript because it's a Yeoman generator under the hood, which has implications on the project layout. One more reason for getting rid of Yeoman! See #844
|
CLI is written in JS not TS! So there's no build for it, which means there is no |
|
@virkt25 Please check why CI fails. |
| "build:current": "lerna run --loglevel=silent build:current", | ||
| "pretest": "npm run build:current", | ||
| "pretest": "npm run build:current && npm run cli:link", | ||
| "cli:link": "cd packages/cli && npm link", |
There was a problem hiding this comment.
I'm okay with the cli:link command, but not having it added to pretest. This means that any time someone runs tests, they're going to lose their existing global installation of the CLI and have it replaced with the locally-linked one. I expect this would cause havoc for contributors down the line.
kjdelisle
left a comment
kjdelisle
left a comment
There was a problem hiding this comment.
Remove the alteration to pretest, IMO.
| "build:current": "lerna run --loglevel=silent build:current", | ||
| "pretest": "npm run build:current", | ||
| "pretest": "npm run build:current && npm run cli:link", | ||
| "cli:link": "cd packages/cli && npm link", |
There was a problem hiding this comment.
I'm okay with the cli:link command, but not having it added to pretest. This means that any time someone runs tests, they're going to lose their existing global installation of the CLI and have it replaced with the locally-linked one. I expect this would cause havoc for contributors down the line.
|
I'm testing this right now to see if this fixes the issue with tests failing on Windows based on http://yeoman.io/authoring/ I'll ping the team if it works and is ready for review. |
virkt25
changed the title
| "build:current": "lerna run --loglevel=silent build:current", | ||
| "pretest": "npm run build:current && npm run cli:link", | ||
| "cli:link": "cd packages/cli/generators && npm link && cd ../../..", | ||
| "cli:link": "cd packages/cli/generators/app && npm link && cd ../controller && npm link && ../extension && npm link && ../project && npm link && cd ../../..", |
There was a problem hiding this comment.
This is completely wrong. Can you explain?
There was a problem hiding this comment.
Please note we don't depend on global installation of generators as @loopback/cli does not use yo.
There was a problem hiding this comment.
I see. I'll have to see if I can get my hands on a windows machine as I see a lot of tests failing on AppVeyor with the following message:
Error: You don't seem to have a generator with the name “loopback4:C:\projects\loopback-next\packages\cli\generators\app” installed.
But help is on the way:
You can see available generators via npm search yeoman-generator or via http://yeoman.io/generators/.
Install them with npm install generator-loopback4:C:\projects\loopback-next\packages\cli\generators\app.
To see all your installed generators run yo without any arguments. Adding the --help option will also show subgenerators.
If yo cannot find the generator, run yo doctor to troubleshoot your system.
at Environment.create (packages\cli\node_modules\yeoman-environment\lib\environment.js:368:9)
at Object.exports.testSetUpGen (packages\cli\test\test-utils.js:15:14)
at Context.it (packages\cli\test\base-generator.js:15:29)
|
I think I know what causes the Windows build issue now. There are a few places that we use
We need to use |
virkt25
changed the title
* test: run cli tests in top level npm test * fix(cli): use path.sep to be platform agnostic
4 participants
Description
CLI Tests aren't run as part of top level
npm test. Test count before change was 609. After this change it is 694.Related issues
Checklist